-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX #20253 : Acceptance Flake (Docker): The Watch A Video button does not open the right page! (Docker Setup) #20273
Conversation
Hi @Akhilesh-max please assign the required reviewer(s) for this PR. Thanks! |
@StephenYu2018 @seanlip PTAL |
Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. Thanks! |
this.clickOn(watchAVideoButton), | ||
]); | ||
|
||
await this.waitForText('Create New Account'); | ||
const url = this.getCurrentUrlWithoutParameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Occasionally, the URL was being fetched before the page had fully loaded, leading to differences with the expected URL. Therefore, made changes to ensure the page is fully loaded before the URL is fetched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what the URL is in the intermediate stage and what it is after the page settles down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One bigger concern.
this.clickOn(watchAVideoButton), | ||
]); | ||
|
||
await this.waitForText('Create New Account'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we waiting for a certain text to be displayed instead of finding the relevant selector and using waitForSelector
on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephenYu2018 Actually, the page we’re navigating to is a Facebook page. Here, we encounter selectors such as class="x1ey2m1c x9f619 xds687c x10l6tqk x17qophe x13vifvy x1ypdohk"
. I guess, these are automatically generated ones and may not be reliable.
Additionally, may be, It will work out without this function as well, but I feel there are cases wherein this function could be helpful.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use the text "Create New Account"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, for cases like this, I think what I would suggest is to just verify that the URL of the new page is correct and that the navigation stabilizes.
The reason I suggest that is because we don't want our tests to break if Facebook (say) changes how they render their page, which is something that's out of our control. Given that, do you think it would be enough to wait for navigation to stabilize?
Unassigning @StephenYu2018 since the review is done. |
Hi @Akhilesh-max, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
@@ -410,6 +410,30 @@ export class BaseUser { | |||
getCurrentUrlWithoutParameters(): string { | |||
return this.page.url().split('?')[0]; | |||
} | |||
|
|||
/** | |||
* This function Waits for a specific text to appear in the body of the page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This function waits"...
this.clickOn(watchAVideoButton), | ||
]); | ||
|
||
await this.waitForText('Create New Account'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use the text "Create New Account"?
); | ||
} catch (error) { | ||
error.message = `Failed to find text "${text}" within ${timeout} ms.`; | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change it to:
throw new error('message');
@@ -674,10 +674,11 @@ export class LoggedInUser extends BaseUser { | |||
throw new Error('The Watch A Video button does not exist!'); | |||
} | |||
await Promise.all([ | |||
this.page.waitForNavigation(), | |||
this.page.waitForNavigation({waitUntil: ['load', 'networkidle2']}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already have this waitUntil, do we still need the new function waitForText in order to fix the flake?
/** | ||
* This function Waits for a specific text to appear in the body of the page. | ||
* This function only waits for rendered text. It does not wait for text in iframes or text that is hidden or not rendered for some reason. | ||
* The text matching is case-sensitive and space-sensitive, so it might not find the text if there are differences in case or white space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised these lines aren't throwing lint errors -- aren't they too long?
Why is prettier not running locally on your submitted code? Can you get this fixed in your development environment?
* @param {string} text - The text to wait for. | ||
* @param {number} [timeout=30000] - The maximum time to wait in milliseconds. Defaults to 30000 (30 seconds). | ||
*/ | ||
async waitForText(text: string, timeout: number = 30000): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForTextToAppearInPageBody
Change timeout to timeoutMsec -- always include units.
@@ -410,6 +410,30 @@ export class BaseUser { | |||
getCurrentUrlWithoutParameters(): string { | |||
return this.page.url().split('?')[0]; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this function -- why isn't "wait for text to be displayed" sufficient? This method would pick up hidden text in the body or div elements etc., which does not match with what the end user would see on the page.
this.clickOn(watchAVideoButton), | ||
]); | ||
|
||
await this.waitForText('Create New Account'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, for cases like this, I think what I would suggest is to just verify that the URL of the new page is correct and that the navigation stabilizes.
The reason I suggest that is because we don't want our tests to break if Facebook (say) changes how they render their page, which is something that's out of our control. Given that, do you think it would be enough to wait for navigation to stabilize?
this.clickOn(watchAVideoButton), | ||
]); | ||
|
||
await this.waitForText('Create New Account'); | ||
const url = this.getCurrentUrlWithoutParameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what the URL is in the intermediate stage and what it is after the page settles down?
Hi @Akhilesh-max, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
PR Pointers